Fix a layout possible miscalculation in `alloc::RawVec` #83706

a1phyr · 2021-03-31T15:09:33Z

A layout miscalculation could happen in RawVec when used with a type whose size isn't a multiple of its alignment. I don't know if such type can exist in Rust, but the Layout API provides ways to manipulate such types. Anyway, it is better to calculate memory size in a consistent way.

rust-highfive · 2021-03-31T15:21:08Z

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

bjorn3 · 2021-03-31T16:22:54Z

I don't know if such type can exist in Rust

I don't think so.

but the Layout API provides ways to manipulate such types.

Makes sense.

Anyway, it is better to calculate memory size in a consistent way.

I found at least one other place that used * size_of::<T>() inside shrink. There may be others.

JohnTitor

This seems reasonable and I agree that we should also take a look at shrink:

rust/library/alloc/src/raw_vec.rs

Lines 470 to 479 in 451e98e

    
           let (ptr, layout) = if let Some(mem) = self.current_memory() { mem } else { return Ok(()) }; 
        
           let new_size = amount * mem::size_of::<T>(); 
        
           let ptr = unsafe { 
        
               let new_layout = Layout::from_size_align_unchecked(new_size, layout.align()); 
        
               self.alloc.shrink(ptr, layout, new_layout).map_err(|_| TryReserveError::AllocError { 
        
                   layout: new_layout, 
        
                   non_exhaustive: (), 
        
               })? 
        
           };

r=me once it's addressed.

JohnCSimon · 2021-07-31T19:40:27Z

Ping from triage:
@a1phyr

r=me once it's addressed.

Can you please address this

a1phyr · 2021-08-02T09:41:58Z

Done, also fixed capacity_from_bytes that did the same assumption.

library/alloc/src/raw_vec.rs

bors · 2021-08-07T04:20:33Z

☔ The latest upstream changes (presumably #87408) made this pull request unmergeable. Please resolve the merge conflicts.

a1phyr · 2021-08-07T10:33:52Z

Rebased

JohnTitor · 2021-08-07T13:49:39Z

Sorry for the delay!
r? @JohnTitor @bors r+

bors · 2021-08-07T13:49:40Z

📌 Commit 03498aa has been approved by JohnTitor

bors · 2022-02-22T02:03:28Z

⌛ Testing commit 5376317 with merge ed9196e48a1c25c95588058b775e6c71cd47eb83...

rust-log-analyzer · 2022-02-22T02:47:46Z

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

.......... (50/60)
.......... (60/60)


/checkout/src/test/rustdoc-gui/search-result-description.goml An exception occured: Failed to launch the browser process!
Inconsistency detected by ld.so: dl-tls.c: 493: _dl_allocate_tls_init: Assertion `listp->slotinfo[cnt].gen <= GL(dl_tls_generation)' failed!


TROUBLESHOOTING: https://github.com/puppeteer/puppeteer/blob/master/docs/troubleshooting.md
== STACKTRACE ==
Error
Error
    at innerRunTestCode (/node-v14.4.0-linux-x64/lib/node_modules/browser-ui-test/src/index.js:468:16)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)

bors · 2022-02-22T02:48:08Z

💔 Test failed - checks-actions

JohnTitor · 2022-02-22T17:14:53Z

Uhm, another spurious failure came up... @bors retry

bors · 2022-02-22T20:50:41Z

⌛ Testing commit 5376317 with merge 5bd1ec3...

bors · 2022-02-23T00:10:11Z

☀️ Test successful - checks-actions
Approved by: JohnTitor
Pushing 5bd1ec3 to master...

rust-timer · 2022-02-23T01:41:53Z

Finished benchmarking commit (5bd1ec3): comparison url.

Summary: This benchmark run shows 54 relevant regressions 😿 to instruction counts.

Average relevant regression: 0.5%
Largest regression in instruction counts: 1.2% on incr-unchanged builds of deeply-nested-async debug

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression

rylev · 2022-02-24T12:40:58Z

@JohnTitor @a1phyr the perf regressions are somewhat small but still there are enough of them to warrant a check in on what's going on. I assume a small perf regression would be expected since we're now strictly doing more (e.g., checked multiplication), but is the magnitude of the regression here expected?

simplify layout calculations in rawvec The use of `Layout::array` was introduced in rust-lang#83706 which lead to a [perf regression](rust-lang#83706 (comment)). This PR basically reverts that change since rust currently only supports stride == size types, but to be on the safe side it leaves a const-assert there to make sure this gets caught if those assumptions ever change.

simplify layout calculations in rawvec The use of `Layout::array` was introduced in #83706 which lead to a [perf regression](rust-lang/rust#83706 (comment)). This PR basically reverts that change since rust currently only supports stride == size types, but to be on the safe side it leaves a const-assert there to make sure this gets caught if those assumptions ever change.

rust-highfive assigned joshtriplett Mar 31, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 31, 2021

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2021

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 4, 2021

bstrie added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 19, 2021

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 5, 2021

crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2021

JohnTitor approved these changes Jun 28, 2021

View reviewed changes

crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 17, 2021

a1phyr force-pushed the fix_vec_layout_calculation branch from 117b686 to fe942c2 Compare August 2, 2021 09:37

JohnTitor reviewed Aug 3, 2021

View reviewed changes

library/alloc/src/raw_vec.rs Outdated Show resolved Hide resolved

a1phyr force-pushed the fix_vec_layout_calculation branch from fe942c2 to e7f1c8e Compare August 7, 2021 10:33

rust-highfive assigned JohnTitor and unassigned joshtriplett Aug 7, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 7, 2021

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 22, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 23, 2022

bors merged commit 5bd1ec3 into rust-lang:master Feb 23, 2022

rustbot added this to the 1.61.0 milestone Feb 23, 2022

rustbot added the perf-regression Performance regression. label Feb 23, 2022

the8472 mentioned this pull request Jan 21, 2023

simplify layout calculations in rawvec #107167

Merged

a1phyr deleted the fix_vec_layout_calculation branch March 12, 2024 17:12

the8472 mentioned this pull request Jun 21, 2024

Apply "polymorphization at home" to RawVec #126793

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a layout possible miscalculation in `alloc::RawVec` #83706

Fix a layout possible miscalculation in `alloc::RawVec` #83706

a1phyr commented Mar 31, 2021

rust-highfive commented Mar 31, 2021

bjorn3 commented Mar 31, 2021

JohnTitor left a comment

JohnCSimon commented Jul 31, 2021 •

edited

Loading

a1phyr commented Aug 2, 2021

bors commented Aug 7, 2021

a1phyr commented Aug 7, 2021

JohnTitor commented Aug 7, 2021

bors commented Aug 7, 2021

bors commented Feb 22, 2022

rust-log-analyzer commented Feb 22, 2022

bors commented Feb 22, 2022

JohnTitor commented Feb 22, 2022

bors commented Feb 22, 2022

bors commented Feb 23, 2022

rust-timer commented Feb 23, 2022

rylev commented Feb 24, 2022

	let (ptr, layout) = if let Some(mem) = self.current_memory() { mem } else { return Ok(()) };
	let new_size = amount * mem::size_of::<T>();

	let ptr = unsafe {
	let new_layout = Layout::from_size_align_unchecked(new_size, layout.align());
	self.alloc.shrink(ptr, layout, new_layout).map_err(\|_\| TryReserveError::AllocError {
	layout: new_layout,
	non_exhaustive: (),
	})?
	};

Fix a layout possible miscalculation in alloc::RawVec #83706

Fix a layout possible miscalculation in alloc::RawVec #83706

Conversation

a1phyr commented Mar 31, 2021

rust-highfive commented Mar 31, 2021

bjorn3 commented Mar 31, 2021

JohnTitor left a comment

Choose a reason for hiding this comment

JohnCSimon commented Jul 31, 2021 • edited Loading

a1phyr commented Aug 2, 2021

bors commented Aug 7, 2021

a1phyr commented Aug 7, 2021

JohnTitor commented Aug 7, 2021

bors commented Aug 7, 2021

bors commented Feb 22, 2022

rust-log-analyzer commented Feb 22, 2022

bors commented Feb 22, 2022

JohnTitor commented Feb 22, 2022

bors commented Feb 22, 2022

bors commented Feb 23, 2022

rust-timer commented Feb 23, 2022

rylev commented Feb 24, 2022

Fix a layout possible miscalculation in `alloc::RawVec` #83706

Fix a layout possible miscalculation in `alloc::RawVec` #83706

JohnCSimon commented Jul 31, 2021 •

edited

Loading